Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Realistic BTOF digitization #1635

Open
wants to merge 90 commits into
base: main
Choose a base branch
from

Conversation

ssedd1123
Copy link

@ssedd1123 ssedd1123 commented Oct 15, 2024

Briefly, what does this PR introduce?

It's an attempt to make LGAD sensor response for Barrel TOF more realistic by doing the following,

  1. Spread charge deposition from strips that are hit directly to nearby strips.
  2. Perform digitization by converting charge deposited to electric pulse.
  3. Digitize the pulse by converting it to TDC and ADC value. ADC propto pulse height and TDC propto time when voltage crosses certain threshold.
  4. Convert the TDC and ADC value back to time and Edep by linear fit.
  5. Return the BTOF hit point as "TOFBarrelRecHit". Position is estimated by either weighted sum of ADC or just center of the cell with max ADC. It's weighted average by default, but the behavior can be changed by parameters.

Noise, edge correction and time talk correction will be developed in the future.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • [x ] Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

It replaces the content of "TOFBarrelRecHit" with results of this new clusterization. It was originally just geant point + random smearing. I have communicated to the reconstruction group and they advise that I submit this pull request so they can study the effect of having this new digitization/clusterization algorithm. They will decide if I should save the data in a new branch or replace the origin content.

ssedd1123 and others added 26 commits July 18, 2024 16:52
… distance relative to the center of the center cell. Now it's changed so spread is calculated relative to the true hit location.
…ighbors when cell ID from BitFieldDecoder for cellID is negative.
@github-actions github-actions bot added topic: tracking Relates to tracking reconstruction topic: PID Relates to PID reconstruction topic: barrel topic: digitization labels Oct 15, 2024
@ssedd1123 ssedd1123 dismissed ruse-traveler’s stale review December 2, 2024 22:17

The requested changes are addressed.

@veprbl
Copy link
Member

veprbl commented Dec 3, 2024

I think this is in good shape now. @simonge, do you agree?

@simonge
Copy link
Contributor

simonge commented Dec 3, 2024

The issues with the functionality of the code have not been addressed yet. It might not be a problem for the majority of datasets being processed at the moment but will be a significant bug and may cause unnecessary confusion down the line for any processing of events distributed throughout time.

@ssedd1123
Copy link
Author

ssedd1123 commented Dec 5, 2024

The issues with the functionality of the code have not been addressed yet. It might not be a problem for the majority of datasets being processed at the moment but will be a significant bug and may cause unnecessary confusion down the line for any processing of events distributed throughout time.

I believe the main conflicts lies in the meaning of tMin. The way I intended tMin and tMax to work is that pulse generation happens at time tMin < t < tMax with no signal outside of this range. The generated pulse will be passed on to the digitization class which convert the pulse to TDC and ADC value.

You proposed that tMin = _DigitizeTime(time) where time = hit.getTime() * dd4hep::ns;, but I fail to understand why that should be the case. If tMin is indeed hit.getTime(), that means the pulse always begin at time (internal EICROC time, not global time) = 0! I don't think that should be the case. If a hit arrives at t (global time) = 5 ns (let's say), then the way I envision my class to work is that for the first 5 ns, the pulse will be flat (i.e. no voltage), and then the voltage increases and decreases according to the landau template pulse.

I understand that collision time will not be zero both due to synchronization issue between the global 100 MHz clock and the local EICROC 40 MHz clock and finite size of bunches, if that's what you want me to implement, I can do it by shifting the tMin of each event by a random number. However, the last time I communicate this idea to you, it seems like that's not what you are asking either.

Another point you raise that I cannot agree is that we cannot discard hit that arrives at t > 25 ns. To the best of my knowledge, that's really how LGAD works in reality where signal that arrives outside beyond the 40 MHz cycle is simply discarded. As a matter of fact, the pulse timing are defined by the 40 MHz clock. It's why I don't believe tMin can be related to the hit time. tMin is when a new cycle of 40 MHz clock starts regardless of when a hit arrives.

You also mentioned that I need to deal with start time alignment. That's only an issue if tMin is not a constant in an event. If tMin = 0, there is no need to worry about mismatch of bin edges because every TDC is binned with respect to the 40 MHz clock regardless of when a hit arrives. That's another reason why I don't think tMin = _DigitizeTime(time) is right.

To be fair, none of the people that I talked to at BNL can firmly confirm or correct my understanding. Since the electronics are still in the design phase, anything can change. What I've said (and programmed in this pull request) is my best guess at what should happen. I program my PulseGenerator this way because I believe it reflects how the detector works in reality.

I believe we disagree on how EICROC work, and not mistakes on programming. If we can't reach an agreement on how time works, then unfortunately there is no way I can continue the development of this digitization code. Will you be interested in a Skype/Zoom meeting sometime next week so we can resolve our misunderstanding?

@Chao1009
Copy link
Contributor

Chao1009 commented Dec 5, 2024

Hi @ssedd1123 and @simonge, I suggest we discuss this digitization issue in the simulation WG meeting on Dec. 17 (Tuesday) at 10 am EST. Would the time work for you? https://indico.bnl.gov/event/23277/

@ssedd1123
Copy link
Author

Hi @ssedd1123 and @simonge, I suggest we discuss this digitization issue in the simulation WG meeting on Dec. 17 (Tuesday) at 10 am EST. Would the time work for you? https://indico.bnl.gov/event/23277/

That works for me. I will prepare a presentation.

@simonge
Copy link
Contributor

simonge commented Dec 6, 2024

Hi @ssedd1123 and @simonge, I suggest we discuss this digitization issue in the simulation WG meeting on Dec. 17 (Tuesday) at 10 am EST. Would the time work for you? https://indico.bnl.gov/event/23277/

That works for me. I will prepare a presentation.

I'm happy to discuss at the meeting too, hopefully discussing in person and providing some time diagrams will clear things up. I realise you suggested this earlier too.

@Chao1009
Copy link
Contributor

Chao1009 commented Dec 6, 2024

Thanks! The dedicated discussion has been scheduled on Dec. 17 (Tuesday) at 10 am EST: https://indico.bnl.gov/event/23277/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: barrel topic: digitization topic: infrastructure topic: PID Relates to PID reconstruction topic: tracking Relates to tracking reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants